[cDAC] Validate contract presence in interface returning functions - #131009
Conversation
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR tightens cDAC target/interface creation by switching from “try” patterns to exception-based creation with cDAC-specific HRESULTs, and by eagerly validating that all contracts required for SOS/IXCLRDataProcess (and related interfaces) are available before publishing those interfaces.
Changes:
- Replace
ContractDescriptorTarget.TryCreateusage withCreate(...)and surface descriptor failures asFormatExceptionwith cDAC-specificCdacHResultscodes. - Add a contract-availability exception hierarchy plus registry support for distinguishing missing vs unrecognized vs intentionally-unsupported contract versions, and introduce
TryValidatefor no-instantiation validation. - Add
CoreCLRContracts.ValidateForDataAccess(...)and call it from interface-creation entrypoints; expand unit coverage for malformed descriptors and contract/version validation behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs | Adds tests for malformed descriptors, contract missing/unrecognized/obsolete behaviors, and eager validation paths. |
| src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs | Updates test contract registry to return structured exceptions and track unsupported versions. |
| src/native/managed/cdac/tests/TestInfrastructure/DumpTestBase.cs | Switches dump tests to exception-based target creation. |
| src/native/managed/cdac/tests/TestInfrastructure/ContractDescriptor/ContractDescriptorBuilder.cs | Updates descriptor builder to emit contract versions and to create targets via Create(...) with a Try-wrapper. |
| src/native/managed/cdac/tests/DataGenerator/TestTarget.cs | Updates generator test registry to new TryGetContract signature and new abstract members. |
| src/native/managed/cdac/scripts/DumpHelpers.cs | Switches dump-inspection helper to exception-based target creation. |
| src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs | Uses Create(...) and adds eager ValidateForDataAccess gating for interface publication. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs | Implements exception-based descriptor read with CdacHResults classification and stricter malformed/not-found distinction. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs | Adds unsupported-version tracking and TryValidate that avoids instantiation/target reads. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs | Adds ValidateForDataAccess and maps validation failures to distinct CdacHResults. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs | Changes public contract retrieval signature and adds TryValidate/RegisterUnsupported. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractNotAvailableException.cs | Introduces public exceptions describing contract availability and validation failures. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CdacHResults.cs | Introduces cDAC-specific HRESULT constants for descriptor/contract validation failures. |
…point)
Intermediate checkpoint of the cDAC 'policy by support' work:
- CdacHResults: CDAC_E_CONTRACT_{NOT_ADVERTISED,UNRECOGNIZED,UNSUPPORTED}
and CDAC_E_DESCRIPTOR_{NOT_FOUND,MALFORMED} (customer-bit failure codes).
- ContractNotAvailableException hierarchy (Missing/Unrecognized/Obsolete)
and ContractValidationException carrying the distinct HRESULT.
- ContractRegistry.TryValidate + CachingContractRegistry no-instantiation
resolution (safe on partial/triage dumps).
- CoreCLRContracts.ValidateForSos eager validation of the SOS surface,
run at CLRDataCreateInstance only when no legacy DAC is supplied.
- ContractDescriptorTarget descriptor-read failures mapped to
CDAC_E_DESCRIPTOR_* via FormatException.HResult.
- Design docs: contract-validation.md + consumer guide.
- Tests in TargetTests.cs.
3200f05 to
0d02967
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs:172
- This change introduces breaking public API surface in cDAC abstractions (e.g., ContractRegistry.TryGetContract signature change, new TryValidate/RegisterUnsupported APIs) and adds new public types (CdacHResults, ContractNotAvailableException and derived exceptions). These need the normal API review/approval linkage (or the new APIs should be made internal if they aren't intended for external consumption).
public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, [NotNullWhen(false)] out System.Exception? failureException) where TContract : IContract;
public TContract GetContract<TContract>() where TContract : IContract
{
if (!TryGetContract(out TContract contract, out System.Exception? failureException))
{
throw failureException ?? new ContractMissingException(TContract.Name);
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs:487
- CreateTargetFromCorDebugDataTarget throws on GetContractDescriptor failure, but the thrown InvalidOperationException doesn't set a cDAC-specific HRESULT. Since entrypoints propagate Exception.HResult and CLRDataCreateInstanceCore already maps this case to CDAC_E_DESCRIPTOR_NOT_FOUND, it seems worth doing the same here so callers can reliably distinguish 'descriptor not found' from other failures.
int hr = contractLocator.GetContractDescriptor(&contractAddress);
if (hr != 0)
{
throw new InvalidOperationException(
$"{nameof(ICLRContractLocator)} failed to fetch the contract descriptor with HRESULT: 0x{hr:x}.");
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs:166
- This change introduces/changes public API surface in the cDAC abstractions (e.g., TryGetContract now returns an exception, new TryValidate/RegisterUnsupported, new public exception types/HRESULT constants). dotnet/runtime policy requires an approved API proposal/issue (api-approved) for new public APIs and breaking signature changes; please link the approval issue in the PR description or make the new surface internal until API review is completed.
/// <summary>
/// Attempts to get an instance of the requested contract for the target.
/// </summary>
/// <typeparam name="TContract">The contract type to retrieve.</typeparam>
/// <param name="contract">
/// When this method returns <see langword="true"/>, contains the requested contract instance; otherwise, <see langword="null"/>.
/// </param>
/// <param name="failureException">
/// When this method returns <see langword="false"/>, contains the exception that describes why the contract could not be retrieved; otherwise, <see langword="null"/>.
/// </param>
/// <returns>
/// <see langword="true"/> if the requested contract is present and was retrieved successfully; <see langword="false"/> if the contract is not present or registered"/>.
/// </returns>
public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, [NotNullWhen(false)] out System.Exception? failureException) where TContract : IContract;
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs:160
- Comment grammar: "Since cDAC is all-or-nothing for some tools debugger" reads like a missing punctuation/word and is hard to parse. Consider clarifying the sentence so future readers understand why IRuntimeMutableTypeSystem is validated eagerly.
// DBI-only in-box contract: used by the DacDbi path (Legacy/Dbi/DacDbiImpl.cs).
// Since cDAC is all-or-nothing for some tools debugger, a target either
// exposes a fully serviceable set of contracts, or we fall back/fail.
Validate<IRuntimeMutableTypeSystem>(registry); // DacDbiImpl.cs (edit-and-continue mutable type system)
src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs:409
- This test comment says GCInfo's creator "reads RuntimeInfo from target memory", but RuntimeInfo_1 reads from contract-descriptor globals (TryReadGlobalString), not target memory. The test is still valid (creator would chain into missing RuntimeInfo), but the comment is misleading.
// GCInfo's creator reads RuntimeInfo from target memory. Advertise GCInfo but omit RuntimeInfo:
// TryValidate must succeed (a creator is registered) without invoking it, whereas a real
// GetContract would chain into RuntimeInfo and fail.
TargetTestHelpers targetTestHelpers = new(arch);
Before the cDAC returns an IXCLRDataProcess, ISosDac*, or CorDB object, eagerly validate that the target actually exposes every contract those interfaces depend on, and fail with a specific, classifiable HRESULT when it doesn't.
ValidateForDataAccess(Target)checks the full set of contracts the data-access surface consumes (direct and transitive), without instantiating them.- CDAC_E_CONTRACT_NOT_ADVERTISED - target lacks a cDAC required contract.